Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve resolver speed #14663

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Improve resolver speed #14663

merged 2 commits into from
Oct 10, 2024

Conversation

x-hgg-x
Copy link
Contributor

@x-hgg-x x-hgg-x commented Oct 10, 2024

What does this PR try to resolve?

This PR improves the resolver speed after investigations from Eh2406/pubgrub-crates-benchmark#6.

How should we test and review this PR?

Commit 1 adds a test showing a slow case in the resolver, where resolving time is cubic over the number of versions of a crate. It can be used for benchmarking the resolver.

Comparison of the resolving time with various values of N=LAST_CRATE_VERSION_COUNT and C=TRANSITIVE_CRATES_COUNT:

N=100 N=200 N=400
C=1 0.25s 0.5s 1.4s
C=2 7s 44s 314s
C=3 12s 77s 537s
C=6 30s 149s 1107s
C=12 99s 447s 2393s

Commit 2 replaces the default hasher with the hasher from rustc-hash, decreasing resolving time by more than 50%.

Performance comparison with the test from commit 1 by setting LAST_CRATE_VERSION_COUNT = 100:

commit duration
master 16s
with rustc-hash 6.7s

Firefox profiles, can be read with https://profiler.firefox.com:
perf.tar.gz

r? Eh2406

@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2024
src/cargo/core/package_id.rs Outdated Show resolved Hide resolved
@x-hgg-x x-hgg-x force-pushed the resolver-perf branch 2 times, most recently from 40f2907 to 6e57306 Compare October 10, 2024 15:20
Copy link
Contributor

@Eh2406 Eh2406 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing one commit at a time. Some small nits about the test.

crates/resolver-tests/tests/resolve.rs Outdated Show resolved Hide resolved
crates/resolver-tests/tests/resolve.rs Outdated Show resolved Hide resolved
crates/resolver-tests/tests/resolve.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Eh2406 Eh2406 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely love the switch to rustc-hash, from my testing this is a huge speed up with no downside.

Still reviewing the other commits.

@epage
Copy link
Contributor

epage commented Oct 10, 2024

Absolutely love the switch to rustc-hash, Eh2406/pubgrub-crates-benchmark#6 (comment) this is a huge speed up with no downside.

Should we split the test and rustc-hash commits out into its own PR and get that merged while we work through nohash?

@x-hgg-x x-hgg-x force-pushed the resolver-perf branch 3 times, most recently from 0bb3838 to 93db5bf Compare October 10, 2024 17:16
@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Oct 10, 2024

I splitted the nohash commits in #14665.

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 10, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 10, 2024

📌 Commit 93db5bf has been approved by Eh2406

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2024
@bors
Copy link
Collaborator

bors commented Oct 10, 2024

⌛ Testing commit 93db5bf with merge 6d679d3...

bors added a commit that referenced this pull request Oct 10, 2024
Improve resolver speed

### What does this PR try to resolve?

This PR improves the resolver speed after investigations from Eh2406/pubgrub-crates-benchmark#6.

### How should we test and review this PR?

Commit 1 adds a test showing a slow case in the resolver, where resolving time is cubic over the number of versions of a crate. It can be used for benchmarking the resolver.

Comparison of the resolving time with various values of `N=LAST_CRATE_VERSION_COUNT` and `C=TRANSITIVE_CRATES_COUNT`:

|      | N=100 | N=200 | N=400 |
|------|-------|-------|-------|
| C=1  | 0.25s | 0.5s  | 1.4s  |
| C=2  | 7s    | 44s   | 314s  |
| C=3  | 12s   | 77s   | 537s  |
| C=6  | 30s   | 149s  | 1107s |
| C=12 | 99s   | 447s  | 2393s |

Commit 2 replaces the default hasher with the hasher from `rustc-hash`, decreasing resolving time by more than 50%.

Performance comparison with the test from commit 1 by setting `LAST_CRATE_VERSION_COUNT = 100`:
| commit          | duration |
|-----------------|----------|
| master          | 16s      |
| with rustc-hash | 6.7s     |

Firefox profiles, can be read with https://profiler.firefox.com:
[perf.tar.gz](https://github.com/user-attachments/files/17318243/perf.tar.gz)

r? Eh2406
@weihanglo
Copy link
Member

weihanglo commented Oct 10, 2024

While it doesn't seem like this part of code is under the risk of hash collision, there is a concern around the other part in Cargo, see #13171 (comment).

I also did some exploration of blake3 as a stable hash algorithm candidate, and blake3 has become a dependency since #14137. Have we benchmarked blake3 as well?

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 10, 2024

All of these hashes are being used exclusively in memory. So the concerns about consistency between builds let alone consistency between architectures are not relevant here. Happy to see experimentation with other algorithms though.

@bors
Copy link
Collaborator

bors commented Oct 10, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 10, 2024
@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Oct 10, 2024

I rebased on master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eh2406 I moved conversion here for us to follow easier.

All of these hashes are being used exclusively in memory. So the concerns about consistency between builds let alone consistency between architectures are not relevant here.

Despite that, the lang team has recently decided to migrate away from weak hash algorithms. I assume that type_id is also mostly an in-memory stuff. Do we need to follow suit?

Copy link
Member

@weihanglo weihanglo Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How easy is it for an attacker to fake a PackageId, and how bad is it when it really happens?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with type_id is that it assumes that if the hash is equal than the object is equal. In its case, it even goes so far as to do an unsafe transmute based on that assumption.

In this PRs case we only use it for a HashMap. If two things spuriously have the same hash than they will be checked by eq. If an attacker could manage to get a lot of things to hash to the same thing then cargo would spend O(n) time finding the right one instead of O(1) amortized.

How easy is it for an attacker to fake a PackageId, and how bad is it when it really happens?

PackageId's are only generated for dependencies. If a dependency is controlled by the attacker, then serious hash collisions are the least of our problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even SipHash is not an choice from t-lang's perspective in that linked issue, which is what Cargo is using now from std. As you said, we have eq so should be even harder to exploit Cargo.

If an attacker could manage to get a lot of things to hash to the same thing then cargo would spend O(n) time finding the right one instead of O(1) amortized.

This might be a problem for companies using either cargo binary or library as a service. I am not sure if we need to take this into account. Without considering this I am 👍🏾 on this PR.

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 10, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 10, 2024

📌 Commit 3d4d48b has been approved by Eh2406

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2024
@bors
Copy link
Collaborator

bors commented Oct 10, 2024

⌛ Testing commit 3d4d48b with merge 7aa7fb1...

@bors
Copy link
Collaborator

bors commented Oct 10, 2024

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 7aa7fb1 to master...

@bors bors merged commit 7aa7fb1 into rust-lang:master Oct 10, 2024
24 checks passed
@x-hgg-x x-hgg-x deleted the resolver-perf branch October 10, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants